fix(producer): extend artifact validation to PNG-sequence and WebM outputs - #3574

Reviewed at 127b3a025e49e921db7ff2cc9643cc0ef1195cff. The coverage extension is right. One change riding along with it weakens an existing gate more broadly than the title suggests, and CI is red. Not approving at this head.

The coverage extension itself is correct

The restructured ternary in renderOrchestrator.ts does what the title says: PNG sequences now reach validate() with an expectation instead of undefined, GIF stays excluded, and the video path is unchanged. validate() branches on this.kind, so the directory path runs assertFrameCountWithinTolerance against files.length and never reaches assertArtifactDuration. I checked that rather than assuming it, because passing expectedDurationSeconds on the PNG branch reads as though a duration is being validated for a directory. It is not, it is simply unused there. Worth a word in the code for the next reader, since the field being present implies a check that does not happen.

Warning when the probe returns nothing instead of silently returning is a real improvement. A gate that skips itself should say so.

The concern: the tolerance change is not scoped to what this PR is extending

shortfall <= 1 becomes shortfall <= Math.max(1, Math.ceil(expectedFrames * 0.002)), and that applies to every artifact kind, including the video path that was already gated and already passing at a tolerance of one frame.

Concretely, on a 10,000 frame render the gate now accepts a 20 frame shortfall as healthy. On 900 frames it accepts 2. This is a truncation detector, so every frame of slack is a frame of corruption it will no longer report.

If PNG sequences need slack because a directory file count and a probed frame count do not agree exactly, that is a good reason for a PNG-specific tolerance. It is not a reason to relax the video gate, which is not what this PR set out to change and which nothing here reports as failing. Scoping the tolerance to the directory path keeps the extension and leaves the existing gate at the strictness it has been holding.

I would rather see that split than approve it bundled. Not calling it a defect, because a deliberate 0.2 percent allowance is a defensible engineering choice. It just is not the choice this PR announces, and a reviewer reading the title would not expect to find it.

Also blocking right now: formatting, and it cascades

All four red checks come from one cause. format:check reports issues in packages/producer/src/services/render/artifactTransaction.test.ts. That fails Format and Preflight (lint + format), which leaves the regression shards skipped, and the regression gate reads skipped != success and fails as well. Running the formatter without --check on that file should clear all four.

Push the formatting fix and either scope the tolerance or say why it should be global, and I will approve at the new head.

Review by Rames

Re-review at 7921f7ce33213dc263865defd6349a274252ecea.

Both things I asked for are done, and the tolerance scoping is done the right way.

The tolerance is now scoped to the PNG-sequence path

assertFrameCountWithinTolerance takes an optional toleranceFrames and falls back to 1 (packages/producer/src/services/render/artifactTransaction.ts:178-194). I read both call sites at this head rather than trusting the diff:

• :256, the directory path this PR extends, passes Math.max(1, Math.ceil(expected.expectedFrames * 0.002)) explicitly.
• :281, the video path inside assertArtifactDuration, passes no tolerance and so inherits the ?? 1 default.

That restores the video gate to exactly the strictness it held before this PR, which was the whole concern: a 10,000 frame render no longer accepts a 20 frame shortfall as healthy on a path nothing here reported as failing. The slack now exists only where the mismatch it compensates for actually lives, a directory file count measured against a probed frame count.

The ?? 1 default is the detail that makes this durable rather than just currently correct. A future call site that forgets the argument inherits the strict gate, not the loose one. Defaulting the other way would have been the easy version of this fix and the wrong one.

The test-file changes in the same commit are Prettier reflow.

Correcting my own earlier framing

My previous review also held on CI being red. I withdraw that reason. A format check is not a code defect, and the merits are what I should have been answering.

On the merits, the coverage extension is correct: the restructured ternary sends PNG sequences into validate() with an expectation instead of undefined, GIF stays excluded, the video path is unchanged, and validate() branches on this.kind so the directory path runs the frame-count assertion and never reaches assertArtifactDuration. Warning when the probe returns nothing, instead of silently passing a gate the caller asked for, is a real improvement.

One line of comment for the next reader is still worth adding: the PNG branch receives expectedDurationSeconds and never uses it, which reads as though a duration is being validated for a directory. Optional.

Why this is a comment and not an approval

Not about the code. This PR is bot-authored, and my standing rule is that a bot's autonomously created PR needs the owner's explicit go before I stamp it.

Auto-merge is armed here as well (miga-heygen, squash). With REVIEW_REQUIRED outstanding, my approval would function as the merge trigger rather than as a review signal, and that call belongs to a human on the team.

Review by Rames